refactor(level-factors): typed run plan + recorded estimand/population metadata (#389 finding 6) - #445
Open
ethanbuckley wants to merge 1 commit into
Open
refactor(level-factors): typed run plan + recorded estimand/population metadata (#389 finding 6)#445ethanbuckley wants to merge 1 commit into
ethanbuckley wants to merge 1 commit into
Conversation
…n metadata (#389 finding 6) Mirrors the ITT / gain-factor (#418) run-plan pattern for the level-factor family. A new level_factors.py provides LevelFactorsModelSettings (fail-fast on a misspelled spec.extra key) and a resolved LevelFactorsRunPlan that drives data preparation, factory construction and the config.json / model_recipe.md audit trail, recording the design, estimand, causal status, analysis population and missing-data assumption alongside every fit -- the level family previously persisted none of that while its report published an unqualified causal claim (#389 finding 4). fit_level_factors now resolves and validates the plan before the context resets an output directory or the loader reads data, then consumes plan.prepare_kwargs() / plan.factory_kwargs(); reporting._resolved_run_plan gains a level_factors branch so write_model_recipe and config.json's resolved_run_plan pick it up. The resolved kwargs reproduce the former inline logic exactly, so every one of the 11 registered level-factor models is byte-identical (build unchanged; drift-guard, factory, key-findings and prior-fallback tests pass; lf-001 dev fit clean and now emits model_recipe.md plus a populated resolved_run_plan). Addresses #389 findings 4 and 6. Findings 1 (t2 estimand methodological decision), 2 (intercept prior recentring), 3 (prior_pushforward + surfaced psense) and 5 (SP/RW naming), plus the 11-model reporting-tier refit, are follow-ups. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 27, 2026
Open
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the statistical-model “typed run plan” pattern to the level_factors family, mirroring the earlier ITT and gain-factors refactors, so level-factor fits resolve/validate settings before loading data and persist a concrete, audited run plan (including estimand/population metadata) into config.json and model_recipe.md.
Changes:
- Added
LevelFactorsModelSettings+LevelFactorsRunPlanandresolve_level_factors_run_plan()to strictly validate/resolve level-factor configuration (including fail-fast rejection of unknown legacyspec.extrakeys). - Updated
fit_level_factorsto drive data preparation and factory construction via the resolved plan, and updated reporting to persist/restore the resolved plan for the level-factors family. - Added comprehensive unit tests covering settings validation, run-plan resolution, and “all registered level-factor models resolve with required metadata” acceptance criteria.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/language_reading_predictors/statistical_models/level_factors.py |
New typed settings + resolved run-plan implementation for the level-factors family, including recorded estimand/population metadata and loader/factory kwargs helpers. |
src/language_reading_predictors/statistical_models/pipeline.py |
Refactors fit_level_factors to resolve/attach the plan before any data loading/output reset, and to use plan.prepare_kwargs() / plan.factory_kwargs(). |
src/language_reading_predictors/statistical_models/reporting.py |
Adds a level-factors branch so resolved_run_plan and model_recipe.md generation can consume/reconstruct the typed plan. |
tests/statistical_models/test_level_factors.py |
New tests validating strict settings translation, plan resolution, and full registered-suite metadata coverage. |
Comment on lines
+198
to
+199
| "Note: Generated from the validated level-factor run plan; template " | ||
| "drafted by an LLM-based AI tool (Claude Code/Opus 4.8).\n\n" |
Comment on lines
+305
to
+309
| "Available-case children observed across the level waves (about 53-54 " | ||
| "depending on outcome). The randomised interpretation applies to the t2 " | ||
| "contrast on this available-case population, not automatically the complete " | ||
| "randomised cohort." | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Drafted by an LLM-based AI tool (Claude Code/Opus 4.8).
Towards #389 (the level-factor critical-review follow-up), mirroring the gain-factor #418 run-plan for the level family. Deliberately not an auto-closing
Closes— this addresses findings 4 and 6; the rest are follow-ups (below).What this does (findings 6 & 4)
A new
level_factors.pygives the family the same typed boundary the ITT and gain-factor families already have:LevelFactorsModelSettings— immutable, slotted, validated;from_legacy_extrarejects unknownspec.extrakeys so a misspelling (group_by_tyme) fails before data loading instead of silently taking a default (finding 6).LevelFactorsRunPlan— resolved and validated before the context resets an output directory or the loader reads data. One object drivesload_and_prepare, the factory call, the teachingmodel_recipe.md, andconfig.json, and records the design, estimand, causal status, analysis population and missing-data assumption (finding 4 — the level family previously persisted null metadata while its report published an unqualified cause-and-effect statement).fit_level_factorsnow consumesplan.prepare_kwargs()/plan.factory_kwargs(effective_adjustment=…), andreporting._resolved_run_plangains alevel_factorsbranch sowrite_model_recipeandconfig.json'sresolved_run_planpick it up.Byte-identical
The resolved kwargs reproduce the former inline
fit_level_factorslogic exactly (samephase_mode="levels", outcome-only load, wave-split covariate timing, factory flags), so the build of every one of the 11 registered level-factor models is unchanged. Verified:lrp-rli-lf-001dev fit is clean (210 obs = 53×4, 0 divergences) and now emitsmodel_recipe.md+ a populatedresolved_run_planinconfig.json;test_level_factors.py): fail-fast on unknown key, typed-vs-legacy source, mixed-declaration rejection, off-floor node/estimand, wave-split timing, effective-adjustment override, and every registered model resolves with recorded metadata.ruff,npm run format:check,npm run spellcheckall clean.Estimand honesty
The run-plan states the t2 quantity as currently implemented and explicitly flags that its precise definition — population-standardised average vs conditional-at-a-profile, and the treatment of the currently time-invariant
group × abilityterm — is the open methodological decision in finding 1. This PR does not pre-empt that decision.Follow-ups (separate PRs / gated)
group × abilityhandling (@frankbuckley's call).prior_pushforward.csv+ surface the t2 psense warning beside key findings.Assigning to a human for review and merge.